Handle 'Content-Length' requests #129

legionth · 2017-02-19T15:36:04Z

Currently Content-Length requests aren't checked for the transferred length.
The new class ensures that only the correct length will be transferred and a correct end event will be emitted.

clue

Nice! Thanks for this high quality PR! 👍

May I ask you to check this wrt to its relation to #116 and #104. In particular, does this resolve #104 or do you see anything that's missing here?

clue · 2017-02-19T15:39:38Z

src/Server.php

+                // Content-Length value is not an integer or not a single integer
+                $this->emit('error', new \Exception('The value of `Content-Length` is not valid'));
+                return;
+            }


LGTM, but should probably also send an error response to client à la #124/#127.

clue · 2017-02-19T15:41:06Z

src/Server.php

@@ -139,6 +139,19 @@ public function handleRequest(ConnectionInterface $conn, Request $request)
            }
        }

+        if ($request->hasHeader('Content-Length')) {


Have you checked this wrt to chunked transfer encoding?

In particular wrt validating requests that contain both chunked transfer encoding AND a Content-Length?

clue · 2017-02-19T15:44:37Z

src/Server.php

+            // stream must emit an 'end' here, because empty data won't be emitted
+            $stream->emit('end');
+            $conn->removeListener('data', array($stream, 'handleData'));
+        }


This block appears a bit unclear to me, perhaps you can clarify? The $contentLength variable is defined in another scope and may not exist here (though this may not cause here due to short-circuit evaluation).

Why is this listener removed (only) here?

legionth · 2017-02-19T20:15:41Z

Ping @clue. I think, I handled all your remarks.

clue · 2017-02-19T21:32:05Z

src/Server.php

+            if ($request->hasHeader('Content-Length')) {
+                // remove 'Content-Length' header accordig to: https://tools.ietf.org/html/rfc7230#section-3.3.3
+                $headers = $request->getHeaders();
+                unset($headers['Content-Length']);


I'm undecided on this (and won't block this), but I'd like to hear some other thoughts on this 👍

How should this be handled? Should this header be removed here?

If a message is received with both a Transfer-Encoding and a
Content-Length header field, the Transfer-Encoding overrides the
Content-Length. Such a message might indicate an attempt to
perform request smuggling (Section 9.5) or response splitting
(Section 9.4) and ought to be handled as an error.

https://tools.ietf.org/html/rfc7230#section-3.3.3

We have a bit more precised specification in the old RFC(https://www.ietf.org/rfc/rfc2616.txt chapter 4.4)

If a message is received with both a
Transfer-Encoding header field and a Content-Length header field,
the latter MUST be ignored.

This would be easier to handle and a more solid behavior.

clue · 2017-02-19T21:32:14Z

src/Server.php

+                    $request->getQueryParams(),
+                    $request->getProtocolVersion(),
+                    $headers
+                );


FWIW, this is not particularly reliable and currently does not set the correct remoteAddress property. (see also above, as I'm not sure this is needed in the first place)

See comment above. I found somethin in the old RFC which might eliminate this behavior.

clue · 2017-02-19T22:02:06Z

src/Server.php

+                return $this->writeError($conn, 400);
+            }
+
+            $stream = new LengthLimitedStream($conn, $contentLength);
        }


What if a request has neither chunked transfer encoding nor a Content-Length header (such as for simple GET requests)? Is this tested?

Should this be part of this PR? (see also #104)

afaik every test with createGetRequest would be effected. The tests for this would be the same, to the ones that already exist.

Not sure I get what you're suggesting here and I may be missing something obvious here, but I'm specifically talking about this:

If this is a request message and none of the above are true, then
the message body length is zero (no message body is present).

https://tools.ietf.org/html/rfc7230#section-3.3.3

Is this correctly covered and tested by this PR or is this not supposed to be part of this PR? See also the PR title and #104.

Edited the first comment. This only closes a part of #104

clue · 2017-02-19T22:03:30Z

src/LengthLimitedStream.php

+        if (!$this->closed) {
+            $this->emit('end');
+            $this->close();
+        }


What if the stream ends before the content length has been reached? (see also chunked transfer encoding)

If a valid Content-Length header field is present [… and] the sender closes the connection […] before the indicated number of octets are
received, the recipient MUST consider the message to be
incomplete and close the connection.

https://tools.ietf.org/html/rfc7230#section-3.3.3

A error handling is missing here. Also in the ChunkedDecoder I would like to keep it out of this PR because it is currently the same state as ChunkedDecoder. I will open a separted PR for this.

See latest commit. An error will be emitted, so the behavior is the same like in ChunkedDecoder. I would prefer to add an error handling in an sperarted PR.

clue · 2017-02-19T22:04:47Z

tests/ServerTest.php

+            $request->on('end', $endEvent);
+            $request->on('close', $closeEvent);
+            $request->on('error', $errorEvent);
+        });


This whole event will not be emitted anyway? Can be simplified a lot, see also other test cases 👍

I wanted to show that the correct events will be used/not used. But ok.

That's exactly what I'm suggesting here :-) The whole request will not be emitted (which is not tested), as such no events can be emitted on the non-existent request object :-)

I think you commented the wrong test, that's why I was so confused. I think you wanted to mark the testNonInteger... tests here. In that case, I agree with you :)

legionth · 2017-02-21T10:30:46Z

Ping @clue . Rebased and squashed some commits. I hope this good to review, if not just say it.

clue

LGTM 👍

WyriHaximus

clue requested changes Feb 19, 2017

View reviewed changes

clue added the new feature label Feb 19, 2017

clue added this to the v0.6.0 milestone Feb 19, 2017

legionth force-pushed the handle-body branch 2 times, most recently from df65f14 to 1fd2812 Compare February 19, 2017 20:13

clue requested changes Feb 19, 2017

View reviewed changes

legionth force-pushed the handle-body branch from 018c48c to e2bdac3 Compare February 21, 2017 10:09

legionth added 4 commits February 21, 2017 11:15

Add LenghtLimitedStream

e873b2c

Handle Content-Length requests

60b7dad

Ignore Content-Length if Transfer-Encoding isset instead of replacing

ff818b5

Handle unexpected end in LengthLimitedStream

39eb913

legionth force-pushed the handle-body branch from e2bdac3 to 39eb913 Compare February 21, 2017 10:15

clue approved these changes Feb 21, 2017

View reviewed changes

jsor approved these changes Feb 21, 2017

View reviewed changes

WyriHaximus approved these changes Feb 22, 2017

View reviewed changes

WyriHaximus merged commit 34acf52 into reactphp:master Feb 22, 2017

legionth mentioned this pull request Mar 2, 2017

Emit end event for requests without body #136

Merged

clue mentioned this pull request Mar 7, 2017

Support detecting request body #104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle 'Content-Length' requests #129

Handle 'Content-Length' requests #129

legionth commented Feb 19, 2017 •

edited

Loading

clue left a comment

clue Feb 19, 2017

clue Feb 19, 2017

clue Feb 19, 2017

legionth commented Feb 19, 2017

clue Feb 19, 2017

legionth Feb 20, 2017

clue Feb 19, 2017

legionth Feb 20, 2017

clue Feb 19, 2017

legionth Feb 19, 2017

clue Feb 20, 2017

legionth Feb 20, 2017

clue Feb 19, 2017

clue Feb 20, 2017

legionth Feb 20, 2017

legionth Feb 21, 2017

clue Feb 19, 2017

legionth Feb 19, 2017

clue Feb 20, 2017

legionth Feb 20, 2017

legionth commented Feb 21, 2017

clue left a comment

WyriHaximus left a comment

Handle 'Content-Length' requests #129

Handle 'Content-Length' requests #129

Conversation

legionth commented Feb 19, 2017 • edited Loading

clue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legionth commented Feb 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legionth commented Feb 21, 2017

clue left a comment

Choose a reason for hiding this comment

WyriHaximus left a comment

Choose a reason for hiding this comment

legionth commented Feb 19, 2017 •

edited

Loading